dev/core#1035 Add in new setting http_timeout to set how long in seco… #14525
Conversation
|
(Standard links)
|
seamuslee001
force-pushed
the
feature_timeout_setting
branch
from
2bb118a to
4ccb0a2
Compare
|
Just pointing out that this change was due to the CiviCRM HTTP request blocking all other processes from executing. So a default of 5 seconds will re-introduce the problem which was trying to be solved. Suggestion, leave a 0.25s default and if it's a problem for some sites they can override it locally. |
seamuslee001
force-pushed
the
feature_timeout_setting
branch
from
4ccb0a2 to
3ac17a5
Compare
|
I checked with John Twyman in AUG and also had a bit of discussion on the AUG team and we figured 1 / 2 might be more closer to a decent timeout so have set it to be 1s. I tend to agree with Justin in that we have known this check to slow down systems and the shorter the better but we have also seen that 0.5s is too short for some sites and 1s seems to be relatively sensible timeout to me |
|
@seamuslee001 thanks for taking on board. What type of internal HTTP request cannot deliver a response in less than 1 second? Genuine question. |
|
@jusfreeman we've had 2 people report it happening to them - not ideal but... I'm happy to try 2 seconds but also I would imagine most partners would override the setting (potentially in civicrm.settings.php ) for their own sites - we have at least retained the ability to do that even if the default feels like an ugly compromise |
settings/Core.setting.php
Outdated
| 'maxlength' => 3, | ||
| ], | ||
| 'default' => 1, | ||
| 'add' => '5.16', |
CRM/Utils/Check/Component.php
Outdated
| * | ||
| * @return bool | ||
| */ | ||
| public function fileExists($url, $timeout = 0.50) { | ||
| public function fileExists($url, $timeout = FALSE) { |
There was a problem hiding this comment.
should we rename it to timeoutOverride?
There was a problem hiding this comment.
@seamuslee001 see above - would that be clearer / more accurate?
There was a problem hiding this comment.
@eileenmcnaughton i don't think so i think its fine just as is
|
@seamuslee001 I'm generally OK with this - my reservations are whether we expose it through the UI initially - or it's just a documented advanced setting that partners etc might use. How about we don't set it through the UI in the rc / patch release variant and we pick that question up against core. There are a couple of issues in setting it through the UI - mostly around clarity of information & risk of reduced stats ping backs that I feel deserve more leisurely consideration |
|
@seamuslee001 discussed with @totten - can you remove from the form for now & leave at 5 seconds & also create a documentation ticket and / or PR & then we will merge |
seamuslee001
force-pushed
the
feature_timeout_setting
branch
from
3ac17a5 to
310b359
Compare
|
updated now @eileenmcnaughton |
|
@eileenmcnaughton @totten documentation PR is here civicrm/civicrm-sysadmin-guide#170 |
|
Jenkins re test this please |
| 'title' => ts('HTTP request timeout'), | ||
| 'is_domain' => 1, | ||
| 'is_contact' => 0, | ||
| 'description' => ts('How long should HTTP requests through Guzzle application run for in seconds'), |
There was a problem hiding this comment.
I'm a bit torn about mentioning guzzle here rather than being more generic - but it could iterate I guess
|
For those like me which are not sure why this PR was created, here is a bit more of a description based on Mattermost chat with Eileen. Suggested amendment to the PR description Have also suggested documentation change, civicrm/civicrm-sysadmin-guide#170 (comment) |
|
thanks @jusfreeman updated i should also note that the previous timeout that was being passed was 0.5 as per the function definition not 0.25. |
|
@seamuslee001 cool man and thanks. Correct 0.5s. |
|
@seamuslee001 I've given this merge on pass but I also have a non-blocking preference that you rename that variable name (per code comment) |
…nds should HTTP requests last for to fix dev/core#1035 Handle situations where a 0 timeout is passed in Remove from form Update variable name as per EIleen
seamuslee001
force-pushed
the
feature_timeout_setting
branch
from
310b359 to
9679fb1
Compare
…nds should HTTP requests last for to fix dev/core#1035
Overview
CiviCRM performs system status checks when the CiviCRM menu route is executed. This can be on every page load. Some of the system status checks execute an internal HTTP request back to the CiviCRM site.
Some users (2 on Stack Exchange URL?) have reported that they are seeing incorrect CiviCRM status check alert failure messages in the CiviCRM UI. This is due to the default timeout being too low and not long enough for their CiviCRM site to complete the individual status check via HTTP.
This PR increases the default from 0.5 seconds to 5 seconds, with the view that this should be sufficient time for most CiviCRM sites to complete the check successfully and not trigger the alert message to the user.
It should be noted that prior to the 0.5 second timeout being introduced, there was no timeout at all and CiviCRM would wait indefinitely until the status check was completed.
From CiviCRM 5.15 onwards there is a hidden setting (http_timeout) for the HTTP request timeout in seconds. The default for this is 5 seconds. Adjusting the timeout will affect both the page load times (lower time, pages will load faster), too low a timeout and CiviCRM status checks may not complete in time resulting in users seeing an incorrect status alert check failed warning message.
Before
No setting and time out was half a second
After
Setting with default timeout of 5s
This adds a setting to allow administrators to set how long HTTP requests should last for, especially for status checks, I have put this against the RC as it is a recent issue / regression on status checks
ping @eileenmcnaughton @totten